Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http2: close idle connections when allowHTTP1 is true #51569

Merged
merged 1 commit into from
Feb 1, 2024

Conversation

xsbchen
Copy link
Contributor

@xsbchen xsbchen commented Jan 26, 2024

Fixes: #51493

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Jan 26, 2024
@marco-ippolito
Copy link
Member

Can you write a test please?

@xsbchen
Copy link
Contributor Author

xsbchen commented Jan 26, 2024

Can you write a test please?

sure, will do later

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@xsbchen
Copy link
Contributor Author

xsbchen commented Jan 31, 2024

@marco-ippolito test case added, plz help to review

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 31, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 31, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@mcollina mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 1, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 1, 2024
@nodejs-github-bot nodejs-github-bot merged commit d1114c4 into nodejs:main Feb 1, 2024
55 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in d1114c4

rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Feb 9, 2024
Fixes: nodejs#51493
PR-URL: nodejs#51569
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
targos pushed a commit that referenced this pull request Feb 15, 2024
Fixes: #51493
PR-URL: #51569
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Feb 19, 2024
Fixes: nodejs#51493
PR-URL: nodejs#51569
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
@marco-ippolito marco-ippolito mentioned this pull request Mar 1, 2024
richardlau pushed a commit that referenced this pull request Mar 25, 2024
Fixes: #51493
PR-URL: #51569
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
Fixes: #51493
PR-URL: #51569
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
@richardlau richardlau mentioned this pull request Mar 25, 2024
eugene1g added a commit to eugene1g/node-http2-timer that referenced this pull request Apr 26, 2024
When Http2SecureServer is configured with `allowHTTP1=true`, it calls
`setupConnectionsTracking` to start monitoring for idle HTTP1
connections. `setupConnectionsTracking` expects to have
`this.connectionsCheckingInterval` property defined, but it does not
exist on `Http2SecureServer`. This `undefined` value is passed to
`setInterval`, which results in `checkConnections` being called on
every tick, creating significant additional load on the server CPU.
The fix is to define `this.connectionsCheckingInterval` on the
Http2SecureServer instance.

Refs: nodejs#51569
nodejs-github-bot pushed a commit that referenced this pull request Apr 28, 2024
When Http2SecureServer is configured with `allowHTTP1=true`, it calls
`setupConnectionsTracking` to start monitoring for idle HTTP1
connections. `setupConnectionsTracking` expects to have
`this.connectionsCheckingInterval` property defined, but it does not
exist on `Http2SecureServer`. This `undefined` value is passed to
`setInterval`, which results in `checkConnections` being called on
every tick, creating significant additional load on the server CPU.
The fix is to define `this.connectionsCheckingInterval` on the
Http2SecureServer instance.

Refs: #51569
PR-URL: #52713
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
aduh95 pushed a commit that referenced this pull request Apr 29, 2024
When Http2SecureServer is configured with `allowHTTP1=true`, it calls
`setupConnectionsTracking` to start monitoring for idle HTTP1
connections. `setupConnectionsTracking` expects to have
`this.connectionsCheckingInterval` property defined, but it does not
exist on `Http2SecureServer`. This `undefined` value is passed to
`setInterval`, which results in `checkConnections` being called on
every tick, creating significant additional load on the server CPU.
The fix is to define `this.connectionsCheckingInterval` on the
Http2SecureServer instance.

Refs: #51569
PR-URL: #52713
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
When Http2SecureServer is configured with `allowHTTP1=true`, it calls
`setupConnectionsTracking` to start monitoring for idle HTTP1
connections. `setupConnectionsTracking` expects to have
`this.connectionsCheckingInterval` property defined, but it does not
exist on `Http2SecureServer`. This `undefined` value is passed to
`setInterval`, which results in `checkConnections` being called on
every tick, creating significant additional load on the server CPU.
The fix is to define `this.connectionsCheckingInterval` on the
Http2SecureServer instance.

Refs: #51569
PR-URL: #52713
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
When Http2SecureServer is configured with `allowHTTP1=true`, it calls
`setupConnectionsTracking` to start monitoring for idle HTTP1
connections. `setupConnectionsTracking` expects to have
`this.connectionsCheckingInterval` property defined, but it does not
exist on `Http2SecureServer`. This `undefined` value is passed to
`setInterval`, which results in `checkConnections` being called on
every tick, creating significant additional load on the server CPU.
The fix is to define `this.connectionsCheckingInterval` on the
Http2SecureServer instance.

Refs: #51569
PR-URL: #52713
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
EliphazBouye pushed a commit to EliphazBouye/node that referenced this pull request Jun 20, 2024
When Http2SecureServer is configured with `allowHTTP1=true`, it calls
`setupConnectionsTracking` to start monitoring for idle HTTP1
connections. `setupConnectionsTracking` expects to have
`this.connectionsCheckingInterval` property defined, but it does not
exist on `Http2SecureServer`. This `undefined` value is passed to
`setInterval`, which results in `checkConnections` being called on
every tick, creating significant additional load on the server CPU.
The fix is to define `this.connectionsCheckingInterval` on the
Http2SecureServer instance.

Refs: nodejs#51569
PR-URL: nodejs#52713
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
When Http2SecureServer is configured with `allowHTTP1=true`, it calls
`setupConnectionsTracking` to start monitoring for idle HTTP1
connections. `setupConnectionsTracking` expects to have
`this.connectionsCheckingInterval` property defined, but it does not
exist on `Http2SecureServer`. This `undefined` value is passed to
`setInterval`, which results in `checkConnections` being called on
every tick, creating significant additional load on the server CPU.
The fix is to define `this.connectionsCheckingInterval` on the
Http2SecureServer instance.

Refs: nodejs#51569
PR-URL: nodejs#52713
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP/2 server.close() is broken with "allowHTTP1" mode
5 participants